Skip to content

Fix test isolation, fast polling, and cleanup of command tests#51

Merged
clavery merged 11 commits intomainfrom
bugfix/unit-test-environment-slow-tests
Jan 15, 2026
Merged

Fix test isolation, fast polling, and cleanup of command tests#51
clavery merged 11 commits intomainfrom
bugfix/unit-test-environment-slow-tests

Conversation

@clavery
Copy link
Copy Markdown
Collaborator

@clavery clavery commented Jan 14, 2026

Summary

This PR addresses three issues with the SDK test suite:

  1. Test Isolation: Tests expecting "no credentials" were failing when developers had real ~/.mobify, dw.json config files or SFCC_* environment variables set

  2. Slow Polling Tests: Site-archive tests were slow (~3000ms each) due to default pollInterval in waitForJob()

  3. Improved Command Test Patterns: Replaced brittle manual parse mocking with cleaner Sinon-based patterns and added integration tests using a test fixture

Changes

Test Isolation

  • Add credentialsFile parameter to ResolveConfigOptions and LoadConfigOptions
  • Update MobifySource to use credentialsFile when provided (overrides ~/.mobify)
  • Add --credentials-file flag with MRT_CREDENTIALS_FILE env var to MrtCommand
    • THIS WAS A FUNCTIONAL GAP ANYWAY
  • Add config-isolation.ts helper to clear SFCC_*/MRT_* env vars for tests for general safety
    • This should only be used for command-related tests. Unit tests of most SDK functions should have proper ways to mock or construct and should be more "pure" (i.e not have side effects and should be based on their arguments; and no SDK functions should normally read environment variables)

Fast Polling Tests

  • Use short pollInterval: 10 for site-archive tests instead of default 3000ms (this can probably be 0 though)
  • Tests now complete in milliseconds instead of seconds

Command Test Improvements

  • Add sinon, @types/sinon, @oclif/test as SDK dev dependencies
  • Create stubParse helper for cleaner parse method mocking (replaces manual type-casted overrides)
  • Refactor command test files to use Sinon instead of manual mocking pattern
  • Add test fixture (test/fixtures/test-cli/) - a mini oclif project for integration testing
  • Update testing skill docs with new patterns
File Action Reason
cartridge-command.test.ts Restored + Sinon Tests cartridgePath, cartridgeOptions, provider runner init, findCartridgesWithProviders
base-command.test.ts Simplified + Sinon Keep getExtraParams, catch tests only
instance-command.test.ts Simplified + Sinon Keep requireX, context tests only
mrt-command.test.ts Simplified + Sinon Keep requireMrtCredentials only
oauth-command.test.ts Simplified + Sinon Keep parseAuthMethods, requireOAuthCredentials
ods-command.test.ts Simplified + Sinon Keep odsClient lazy init tests only

Integration Tests (API Contract Validation)

Integration tests validate that base commands work correctly when used the way consumers use them - catching issues that unit tests with mocked parse() cannot:

Issue Type Unit Test Integration Test
Flag definition errors (wrong type, missing env) ❌ Mocked ✅ Oclif fails to parse
baseFlags inheritance broken ❌ Mocked ✅ Flags not recognized
Command lifecycle bugs ⚠️ Partial ✅ Full init→run→serialize

Fixture commands:

  • test-base.js - BaseCommand (extra-query, extra-body flags)
  • test-instance.js - InstanceCommand (server, instance flags)
  • test-mrt.js - MrtCommand (api-key, project, environment, cloud-origin flags)

Integration tests: 18 total (5 BaseCommand + 5 InstanceCommand + 8 MrtCommand)

Documentation

  • Update testing skill with config isolation patterns, pollInterval guidance
  • Add command test guidelines (stubParse helper, integration test fixture pattern)
  • Add guidance on what to test vs avoid in command tests

Test plan

  • All 643 SDK tests pass (including 18 integration tests)
  • Linting passes
  • Site-archive tests complete quickly (not ~3 seconds each)
  • Tests pass regardless of developer's local ~/.mobify configuration
  • cartridge-command.ts coverage improved from 19% to 91%

Test Isolation:
- Add `credentialsFile` parameter to ResolveConfigOptions and LoadConfigOptions
- Update MobifySource to use credentialsFile when provided (overrides ~/.mobify)
- Add --credentials-file flag with MRT_CREDENTIALS_FILE env var to MrtCommand
- Add config-isolation.ts helper to clear SFCC_*/MRT_* env vars for tests

Fast Polling Tests:
- Use short pollInterval for site-archive tests instead of default 3000ms
- Tests now complete in milliseconds instead of seconds

Command Test Cleanup:
- Delete cartridge-command.test.ts (100% trivial delegation tests)
- Simplify base-command.test.ts (keep getExtraParams, catch tests)
- Simplify instance-command.test.ts (keep requireX, context tests)
- Simplify mrt-command.test.ts (keep requireMrtCredentials only)
- Simplify oauth-command.test.ts (keep parseAuthMethods, requireOAuthCredentials)
- Simplify ods-command.test.ts (keep odsClient lazy init tests)

Documentation:
- Update testing skill with config isolation, pollInterval patterns
- Add command test guidelines (what to test vs avoid)
- Remove general knowledge content (basic Mocha/Chai patterns)
@clavery clavery force-pushed the bugfix/unit-test-environment-slow-tests branch from 52c91db to aafef91 Compare January 14, 2026 22:53
- Change loadDwJson() default to look only at ./dw.json (no parent search)
- Update DwJsonSource to use same logic, remove findDwJson import
- Keep findDwJson() exported for users who need explicit upward search
- Set SFCC_CONFIG=/dev/null and MRT_CREDENTIALS_FILE=/dev/null in isolateConfig()
- Update test to verify new behavior (no upward search)
Add isolateConfig()/restoreConfig() to command test files to ensure
tests are isolated from developer's environment variables (SFCC_*, MRT_*).

Files updated:
- test/cli/base-command.test.ts
- test/cli/instance-command.test.ts
- test/cli/mrt-command.test.ts
- test/cli/oauth-command.test.ts
- test/cli/ods-command.test.ts
@clavery clavery marked this pull request as ready for review January 14, 2026 23:42
- Add sinon, @types/sinon, @oclif/test as SDK dev dependencies
- Create stubParse helper for cleaner parse method mocking
- Refactor 5 command test files to use Sinon instead of manual mocking
- Add test fixture (test/fixtures/test-cli/) for integration testing
- Add base-command.integration.test.ts with runCommand() tests
- Update testing skill docs with new patterns

The stubParse helper eliminates the brittle MockableXxxCommand type
casting pattern. Integration tests exercise full command lifecycle
through the oclif test utilities.
- Add cartridge-command.test.ts with tests for cartridgePath,
  cartridgeOptions, provider runner init, and findCartridgesWithProviders
- Use stubParse helper with server mock for instance-dependent tests
- Improves cartridge-command.ts coverage from 19% to 91%
Integration tests provide API contract validation beyond code coverage:
- Catch flag definition errors (wrong type, missing env var)
- Validate baseFlags inheritance works correctly
- Exercise full oclif command lifecycle (discovery, parse, init, run)
- Test commands the way consumers actually use them

New fixtures:
- test-instance.js: Tests server, instance flags and hasServer check
- test-mrt.js: Tests api-key, project, environment, cloud-origin flags

New integration tests:
- instance-command.integration.test.ts (5 tests)
- mrt-command.integration.test.ts (8 tests)

Total integration tests: 18 (BaseCommand + InstanceCommand + MrtCommand)
@clavery clavery changed the title Fix test isolation, fast polling, and cleanup low-value command tests Fix test isolation, fast polling, and cleanup of command tests Jan 15, 2026
charithaT07
charithaT07 previously approved these changes Jan 15, 2026
Copy link
Copy Markdown
Collaborator

@charithaT07 charithaT07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for identifying and addressing the root causes of the test failures, this was a tricky one because the failures only surfaced for some developers due to local config files and environment variables. The new test patterns using sinon, @oclif/test, the stubParse helper, and the fixture CLI project provide a cleaner approach for testing command behavior without brittle overrides. I’ll review the other SDK test files to align them with these updated patterns and ensure consistency across the suite. Thanks again for driving this .

Resolve conflict in base-command.test.ts by:
- Keeping test helper utilities (config-isolation, stub-parse) from feature branch
- Adding globalMiddlewareRegistry cleanup from main
- Adding extra-headers tests from main using feature branch patterns
@clavery clavery merged commit d9b8ddf into main Jan 15, 2026
5 checks passed
@clavery clavery deleted the bugfix/unit-test-environment-slow-tests branch January 27, 2026 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants